Skip to content

chore: rename LiveLocationShares to RoomLiveLocationService#6446

Open
Velin92 wants to merge 4 commits intomainfrom
mauroromito/rename_lls_shares
Open

chore: rename LiveLocationShares to RoomLiveLocationService#6446
Velin92 wants to merge 4 commits intomainfrom
mauroromito/rename_lls_shares

Conversation

@Velin92
Copy link
Copy Markdown
Member

@Velin92 Velin92 commented Apr 14, 2026

name says it all. The previous name was just too confusing, and since this object acts as a service for the room to publish live location share updates, I think the RoomLiveLocationService is a more appropriate name

@Velin92 Velin92 requested a review from a team as a code owner April 14, 2026 12:03
@Velin92 Velin92 requested review from poljar and removed request for a team April 14, 2026 12:03
Copy link
Copy Markdown
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for caring about the SDK cleanness.

A Service is rather a high-level API.

Since the type aims at providing subscription or initial value, I propose the name ObservableLiveLocation to match the other places of the SDK using a similar pattern.

Thoughts?

@Velin92
Copy link
Copy Markdown
Member Author

Velin92 commented Apr 14, 2026

Thanks for caring about the SDK cleanness.

A Service is rather a high-level API.

Since the type aims at providing subscription or initial value, I propose the name ObservableLiveLocation to match the other places of the SDK using a similar pattern.

Thoughts?

What about RoomLiveLocationObserver ? or LiveLocationObserver ?
ObservableLiveLocation feels a bit misleading because this object is not a LiveLocation but something that keeps track of them.

@Hywan
Copy link
Copy Markdown
Member

Hywan commented Apr 14, 2026

I'm good with LiveLocationObserver too. Ideally, it should live inside the matrix_sdk::room module, but let's not do too much change at once 😛.

@Hywan Hywan removed the request for review from poljar April 14, 2026 12:30
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 14, 2026

Merging this PR will not alter performance

✅ 50 untouched benchmarks


Comparing mauroromito/rename_lls_shares (79c2c41) with main (b9b40c0)

Open in CodSpeed

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.81%. Comparing base (9d39e4b) to head (79c2c41).
⚠️ Report is 17 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6446      +/-   ##
==========================================
- Coverage   89.82%   89.81%   -0.02%     
==========================================
  Files         378      378              
  Lines      104314   104314              
  Branches   104314   104314              
==========================================
- Hits        93702    93686      -16     
- Misses       7020     7025       +5     
- Partials     3592     3603      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment thread bindings/matrix-sdk-ffi/src/room/mod.rs Outdated
@stefanceriu
Copy link
Copy Markdown
Member

A Service is rather a high-level API.

Imho this component has potential for growing, by moving the send_live_location method from the Room here and potentially caching the "local echoes". I would just name it service to being with and avoid later complications. Service also makes is consistent with our other services and set expectations on lifetime management correctly.

@Hywan
Copy link
Copy Markdown
Member

Hywan commented Apr 14, 2026

I let you decide then 😃.

@Velin92
Copy link
Copy Markdown
Member Author

Velin92 commented Apr 14, 2026

in the end we decided to rename it to LiveLocationsObserver

Comment thread crates/matrix-sdk/tests/integration/room/beacon/mod.rs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants